Publicize: Move the publicize-connections endpoint to the package #40637

pablinos · 2024-12-16T23:36:58Z

This is following on from various discussions we've been having about how to refactor the API endpoints for connections management. This is a variation on the approach in #40607 but repurposes the existing endpoint rather than creating a new backwards compatible one.

Fixes #

Proposed changes:

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Go to '..'

This is following on from various discussions we've been having about how to refactor the API endpoints for connections management. This is a variation on the approach in #40607 but repurposes the existing endpoint rather than creating a new backwards compatible one.

github-actions · 2024-12-16T23:37:59Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the try/move-publicize-connections-endpoint branch.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack try/move-publicize-connections-endpoint

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2024-12-16T23:38:02Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
🔴 Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
- A. Team Review: add the "[Status] Needs Team Review" label
  - For most changes, including minor cross-team impacts.
  - Example: Updating a team-specific component or a small change to a shared library.
- B. Crew Review: add the "[Status] Needs Review" label
  - For significant changes to core functionality.
  - Example: Major updates to a shared library or complex features.
- C. Both: Start with Team, then request Crew
  - For complex changes or when you need extra confidence.
  - Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

WordPress.com Simple releases happen semi-continuously (PCYsg-Jjm-p2).
WoA releases happen weekly.
Releases to self-hosted sites happen monthly. The next release is scheduled for January 7, 2025 (scheduled code freeze on undefined).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

projects/packages/publicize/src/class-wpcom-rest-api-v2-endpoint-list-publicize-connections.php

projects/packages/publicize/src/class-publicize-setup.php

projects/packages/publicize/src/class-wpcom-rest-api-v2-endpoint-list-publicize-connections.php

manzoorwanijk

I think we have gone back and forth too many times and I don't think we should continue doing that. Let us get this PR in its final shape addressing the comments I added.

I am not sure about your approach about the endpoint initialization on WPCOM, so I will wait for your thoughts on that.

projects/packages/publicize/src/class-wpcom-rest-api-v2-endpoint-list-publicize-connections.php

pablinos

This has turned into a bit of a monster, which is a shame. I was hoping we could do this with smaller changes. It's also almost the same as #40607, but I suppose that means we've converged on a solution.

pablinos · 2024-12-17T19:21:27Z

projects/packages/publicize/src/rest-api/class-connections-controller.php

@@ -0,0 +1,431 @@
+<?php // phpcs:ignore WordPress.Files.FileName.InvalidClassFileName


The annoying thing about this change is that this file is now so different from the publicize-connections.php file it replaces, it's now a new file, rather than the diff showing that it's moved. Overall it is using the same code and method names.

pablinos · 2024-12-17T19:23:46Z

projects/packages/publicize/src/rest-api/class-connections-controller.php

+	 * @return array Connection objects in the same shape as from `get_connections`
+	 */
+	public static function get_connections_data( $run_tests = false ) {
+		// TODO decide on caching


I'm guessing we should use a transient like we do in Publicize?

Caching should ideally happen outside of the controller. We can deal with it later.

pablinos · 2024-12-17T19:26:19Z

projects/packages/publicize/src/rest-api/class-connections-controller.php

+		$response = Client::wpcom_json_api_request_as_user( sprintf( $path, $site_id ), 'v2', array( 'method' => 'GET' ) );
+
+		if ( is_wp_error( $response ) || 200 !== wp_remote_retrieve_response_code( $response ) ) {
+			// TODO log error.


I'm trying to work out what's best to do here. I feel like we should return an error. If it's empty like this then everything will still function but will be blank. How do we get an error message to the user? Maybe it's ok for now

We should always return an array here and pass the error in some other way like JITM.

manzoorwanijk · 2024-12-18T03:41:19Z

projects/packages/publicize/src/class-publicize-base.php

-		$cmeta = $this->get_connection_meta( $connection );
+		$cmeta    = $this->get_connection_meta( $connection );
+		$username = '';


Since username field is deprecated in favour of external_handle, we don't need to update it. We are going to remove it anyway in the future.

manzoorwanijk · 2024-12-18T03:44:36Z

It's funny that this PR now has more changes (+473 −307) than #40607 (+459 −31) which it tried not to do 😄

manzoorwanijk · 2024-12-18T03:46:44Z

projects/packages/publicize/src/rest-api/class-connections-controller.php

+			),
+		);
+
+		$test_fields = array(


Can we please avoid adding these test fields here? We no longer use them. The only field that we need here is the status field.

We can move this schema back to connection test results endpoint and remove it in future.

We do need more than the status field, or we need to make sure the status field can also be must_reauth for LinkedIn. I think that flow does use some of these other fields too, but it needs some work anyway.

Our new connections management uses only the status field, nothing else.

manzoorwanijk · 2024-12-18T03:50:02Z

projects/packages/publicize/src/rest-api/class-connections-controller.php

+			'test_success'  => 'connectionTestPassed',
+			'test_message'  => 'connectionTestMessage',
+			'error_code'    => 'connectionTestErrorCode',
+			'can_refresh'   => 'userCanRefresh',
+			'refresh_text'  => 'refreshText',
+			'refresh_url'   => 'refreshURL',
+			'connection_id' => 'connectionID',


I don't like these unused fields in this class.

Fair enough, we can move them back to the subclass.

manzoorwanijk · 2024-12-18T03:51:20Z

projects/packages/publicize/src/rest-api/class-connections-controller.php

+	 *
+	 * @return true|WP_Error
+	 */
+	public function get_items_permission_check() {


This callback in the future can be moved to a base class to share between different publicize controllers.

Yes, I was trying to avoid introducing a base class at this stage. I was also trying to make the change a move of the endpoint, but we end up having to make so many other changes to the class, it ends up getting treated as a new file

manzoorwanijk · 2024-12-18T04:03:32Z

projects/packages/publicize/src/rest-api/class-connections-controller.php

+					array(
+						'connection_id'        => $connection_id,
+						'display_name'         => $publicize->get_display_name( $service_name, $connection ),
+						'external_handle'      => $username,


external_handle handle is not actually the username. It's a little different. With this PR, I get this, which is incorrect:

You can see the get_external_handle method I added in #40607

Yes it seems strange. Why does it only exist for certain networks? https://github.com/Automattic/jetpack/pull/40607/files#diff-540cd16120af78cc4afc265ca572d7dac4674ebd81ec12f9479bf079929dc04aR612-R629?

Because we don't have it for Facebook pages for example. We can surely improve on that to ensure that our keyring service has all that information.

manzoorwanijk

Apart from many other issues that I pointed out

The initialization on Simple sites is yet to be taken care of
The shape of the data is incorrect. It should be an array of connections, not an array of REST responses.

pablinos · 2024-12-18T06:12:40Z

The initialization on Simple sites is yet to be taken care of

That is taken care of in the accompanying WPCOM PR

The shape of the data is incorrect. It should be an array of connections, not an array of REST responses.

Yes, we should fix that. It was introduced from trying to make Phan happy. I'm not sure we should be using the prepare_item_for_response method

pablinos · 2024-12-18T12:46:16Z

Closing in favour of #40607

pablinos added [Status] In Progress [Package] Publicize labels Dec 16, 2024

pablinos self-assigned this Dec 16, 2024

github-actions bot added [Feature] Publicize Now Jetpack Social, auto-sharing [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Dec 16, 2024

pablinos mentioned this pull request Dec 16, 2024

Move wpcom/v2/publicize/connections endpoint to publicize package #40607

Merged

3 tasks

manzoorwanijk reviewed Dec 17, 2024

View reviewed changes

projects/packages/publicize/src/class-wpcom-rest-api-v2-endpoint-list-publicize-connections.php Outdated Show resolved Hide resolved

manzoorwanijk reviewed Dec 17, 2024

View reviewed changes

projects/packages/publicize/src/class-publicize-setup.php Outdated Show resolved Hide resolved

manzoorwanijk reviewed Dec 17, 2024

View reviewed changes

projects/packages/publicize/src/class-wpcom-rest-api-v2-endpoint-list-publicize-connections.php Outdated Show resolved Hide resolved

manzoorwanijk requested changes Dec 17, 2024

View reviewed changes

manzoorwanijk reviewed Dec 17, 2024

View reviewed changes

projects/packages/publicize/src/class-wpcom-rest-api-v2-endpoint-list-publicize-connections.php Outdated Show resolved Hide resolved

pablinos changed the title ~~Publicize: Mave the publicize-connections endpoint to the package~~ Publicize: Move the publicize-connections endpoint to the package Dec 17, 2024

pablinos added 7 commits December 17, 2024 10:47

Rename the endpoint class

2d94338

Make the get_connections methods static

eb7e188

Only use the connection meta in order to fix a bug.

24d27c1

Correct class name

78afbc0

Add test connections functionality

64e2cfd

Adapt publicize-connection-test-results to use the updated code

684d5c7

Call get_connections as a static method

e75b93c

pablinos commented Dec 17, 2024

View reviewed changes

pablinos added 2 commits December 17, 2024 20:28

Ignore Phan error

47c2044

Implement schema caching

f1d36ef

pablinos marked this pull request as ready for review December 17, 2024 20:31

pablinos added [Status] Needs Team Review and removed [Status] In Progress labels Dec 17, 2024

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Dec 17, 2024

Fix Phan errors

cf9e957

manzoorwanijk reviewed Dec 18, 2024

View reviewed changes

manzoorwanijk requested changes Dec 18, 2024

View reviewed changes

pablinos closed this Dec 18, 2024

github-actions bot removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs Team Review labels Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Publicize: Move the publicize-connections endpoint to the package #40637

Publicize: Move the publicize-connections endpoint to the package #40637

pablinos commented Dec 16, 2024

github-actions bot commented Dec 16, 2024 •

edited

Loading

github-actions bot commented Dec 16, 2024

manzoorwanijk left a comment

pablinos left a comment

pablinos Dec 17, 2024

pablinos Dec 17, 2024

manzoorwanijk Dec 18, 2024

pablinos Dec 17, 2024

manzoorwanijk Dec 18, 2024

manzoorwanijk Dec 18, 2024

manzoorwanijk commented Dec 18, 2024

manzoorwanijk Dec 18, 2024

pablinos Dec 18, 2024

manzoorwanijk Dec 18, 2024

manzoorwanijk Dec 18, 2024

pablinos Dec 18, 2024

manzoorwanijk Dec 18, 2024

pablinos Dec 18, 2024

manzoorwanijk Dec 18, 2024

pablinos Dec 18, 2024 •

edited

Loading

manzoorwanijk Dec 18, 2024

manzoorwanijk left a comment •

edited

Loading

pablinos commented Dec 18, 2024

pablinos commented Dec 18, 2024

		@@ -0,0 +1,431 @@
		<?php // phpcs:ignore WordPress.Files.FileName.InvalidClassFileName

Publicize: Move the publicize-connections endpoint to the package #40637

Publicize: Move the publicize-connections endpoint to the package #40637

Conversation

pablinos commented Dec 16, 2024

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Dec 16, 2024 • edited Loading

github-actions bot commented Dec 16, 2024

manzoorwanijk left a comment

Choose a reason for hiding this comment

pablinos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manzoorwanijk commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablinos Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manzoorwanijk left a comment • edited Loading

Choose a reason for hiding this comment

pablinos commented Dec 18, 2024

pablinos commented Dec 18, 2024

github-actions bot commented Dec 16, 2024 •

edited

Loading

pablinos Dec 18, 2024 •

edited

Loading

manzoorwanijk left a comment •

edited

Loading